Skip to content

Conversation

@jsemldonado
Copy link
Contributor

Pull Request

NautilusTrader prioritizes correctness and reliability, please follow existing patterns for validation and testing.

  • I have reviewed the CONTRIBUTING.md and followed the established practices

Summary

Add event_slug_builder support to PolymarketInstrumentProvider, enabling efficient loading of niche, reoccurring/related prediction markets (e.g., up or down crypto markets, weather markets) by dynamically generating event slugs instead of downloading all 151k+ markets.

Related Issues/PRs

N/A

Type of change

  • Bug fix (non-breaking)
  • New feature (non-breaking)
  • Improvement (non-breaking)
  • Breaking change (impacts existing behavior)
  • Documentation update
  • Maintenance / chore

Breaking change details (if applicable)

N/A

Documentation

  • Documentation changes follow the style guide (docs/developer_guide/docs.md)

Release notes

  • I added a concise entry to RELEASES.md that follows the existing conventions (when applicable)

Testing

Ensure new or changed logic is covered by tests.

  • Affected code paths are already covered by the test suite
  • I added/updated tests to cover new or changed logic

Added 6 test cases in tests/integration_tests/adapters/polymarket/test_providers.py:

  • test_load_all_async_uses_event_slug_builder_when_configured
  • test_event_slug_builder_takes_priority_over_gamma_markets
  • test_event_slug_builder_handles_multiple_slugs
  • test_event_slug_builder_handles_empty_slug_list
  • test_event_slug_builder_continues_on_event_not_found
  • test_event_slug_builder_skips_markets_without_condition_id

Added example scripts in examples/live/polymarket/:

  • slug_builders.py - Example slug builder functions for BTC/ETH/crypto 15-minute UpDown markets
  • polymarket_slug_builder_tester.py - TradingNode example demonstrating the feature

Run with:

python examples/live/polymarket/polymarket_slug_builder_tester.py

@jsemldonado jsemldonado force-pushed the feature/polymarket-event-slug-builder branch from 7205bb5 to cae1f3f Compare January 28, 2026 12:19
@jsemldonado jsemldonado force-pushed the feature/polymarket-event-slug-builder branch from cae1f3f to ca593ff Compare January 28, 2026 12:20
@cjdsellers
Copy link
Member

Hi @jsemldonado

Thanks for the PR, this looks like a valuable feature and well implemented overall. I'll have capacity to review more closely next week.

In the mean time, here are some minor items worth considering:

  1. Inconsistent config attribute pattern (providers.py:109)
  self.config: PolymarketInstrumentProviderConfig | None = config
  1. The class already inherits self._config from the parent. Adding self.config separately creates two attributes pointing to the same data, which is
  confusing. The code uses both self.config (line 123-124) and self._config (line 125).

  1. Recommendation: Use self._config consistently and add a property if needed for type narrowing.
  2. Redundant assertions (providers.py:142-144)
  assert self.config is not None
  assert self.config.event_slug_builder is not None
  2. These conditions are already guaranteed by the caller check on line 123. If validation is needed, Nautilus typically uses PyCondition class rather
  than assert.
  3. Broad exception handling (providers.py:178-182)
  except Exception as e:
      self._log.error(f"Failed to load event slug '{slug}': {e}")
  3. Catching bare Exception can mask unexpected errors. Consider catching more specific exceptions or at least logging the full traceback.
  4. Unused filters parameter (providers.py:131)
  The _load_from_event_slugs method accepts filters but never uses it. Either remove it or apply filters consistently.

  📝 Minor Suggestions

  1. The filters parameter could be used to pre-filter loaded instruments for consistency with other loading paths
  2. Consider logging at DEBUG level for individual slug loading instead of INFO, to reduce log noise when many slugs are processed
  3. The docstring for PolymarketInstrumentProviderConfig is good but could mention that load_all=True is required to trigger the builder (as noted in the
  example)

Also, it would be ideal to add some additional test coverage (just the high-value tests listed in "missing coverage"):

Test Coverage Analysis

  Current Coverage (6 tests)
  ┌─────────────────────────────────────────────────────────────┬────────────────────────────────────┐
  │                            Test                             │          Scenario Covered          │
  ├─────────────────────────────────────────────────────────────┼────────────────────────────────────┤
  │ test_load_all_async_uses_event_slug_builder_when_configured │ ✅ Basic happy path                │
  ├─────────────────────────────────────────────────────────────┼────────────────────────────────────┤
  │ test_event_slug_builder_takes_priority_over_gamma_markets   │ ✅ Priority over use_gamma_markets │
  ├─────────────────────────────────────────────────────────────┼────────────────────────────────────┤
  │ test_event_slug_builder_handles_multiple_slugs              │ ✅ Multiple slugs                  │
  ├─────────────────────────────────────────────────────────────┼────────────────────────────────────┤
  │ test_event_slug_builder_handles_empty_slug_list             │ ✅ Empty slug list                 │
  ├─────────────────────────────────────────────────────────────┼────────────────────────────────────┤
  │ test_event_slug_builder_continues_on_event_not_found        │ ✅ ValueError handling             │
  ├─────────────────────────────────────────────────────────────┼────────────────────────────────────┤
  │ test_event_slug_builder_skips_markets_without_condition_id  │ ✅ Missing conditionId             │
  └─────────────────────────────────────────────────────────────┴────────────────────────────────────┘
  
Missing Coverage
  ┌────────────────────────────────────────────────┬────────┬─────────────────────────────────────────────────┐
  │                      Gap                       │  Risk  │                    Code Path                    │
  ├────────────────────────────────────────────────┼────────┼─────────────────────────────────────────────────┤
  │ Invalid slug builder path (module not found)   │ High   │ resolve_path() raises ModuleNotFoundError       │
  ├────────────────────────────────────────────────┼────────┼─────────────────────────────────────────────────┤
  │ Invalid slug builder path (function not found) │ High   │ resolve_path() raises AttributeError            │
  ├────────────────────────────────────────────────┼────────┼─────────────────────────────────────────────────┤
  │ Slug builder raises exception                  │ Medium │ Exception in slug_builder() call                │
  ├────────────────────────────────────────────────┼────────┼─────────────────────────────────────────────────┤
  │ Generic exception during fetch                 │ Medium │ except Exception branch at line 180             │
  ├────────────────────────────────────────────────┼────────┼─────────────────────────────────────────────────┤
  │ Empty token_id in token_info                   │ Low    │ if not token_id branch at line 169-172          │
  ├────────────────────────────────────────────────┼────────┼─────────────────────────────────────────────────┤
  │ Factory integration                            │ Medium │ instrument_config attribute wiring in factories │
  ├────────────────────────────────────────────────┼────────┼─────────────────────────────────────────────────┤
  │ _log_warnings=False behavior                   │ Low    │ Warning suppression path                        │
  └────────────────────────────────────────────────┴────────┴─────────────────────────────────────────────────┘
  Verdict

  Coverage is decent for the happy paths but has gaps in error handling.

  The most critical missing tests are:

  1. Invalid path resolution - If a user misconfigures event_slug_builder with a typo like "mymodule:build_slugz", the code will crash with an unhandled
  exception. This should either be tested to confirm the behavior or the code should catch and handle it gracefully.
  2. Factory integration - The new instrument_config attribute replaces instrument_provider in the factories, but there are no tests verifying that the
  factory correctly passes this config to the provider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants